-
Notifications
You must be signed in to change notification settings - Fork 13.7k
std: sys: net: uefi: tcp: Initial TcpListener support #145339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
Passing this along to someone else, who hopefully has more context. r? libs |
@rustbot label +O-UEFI |
Thom is off the review rotation Also gentle ping @nicholasbishop for a review since we usually defer UEFI to you |
temp.configure(false, None, Some(x))?; | ||
Ok(Tcp::V4(temp)) | ||
} | ||
SocketAddr::V6(_) => todo!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than todo!()
, maybe add an IPV6_UNSUPPORTED
error to
rust/library/std/src/io/error.rs
Line 78 in b3cfb8f
impl Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to return normal unsupported instead of adding a new error variant. Since IPv6 is just not implemented by std right now. It is supported by UEFI. And I am planning to implement it once IPv4 is complete.
} else { | ||
(DEFAULT_ADDR, 0) | ||
}; | ||
let subnet_mask = helpers::ipv4_to_r_efi(crate::net::Ipv4Addr::new(255, 255, 255, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change? If the mask was previously 0.0.0.0 (all addresses), it seems like this may surprise some users
Worth a comment in any case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not a breaking change. That is because the docs state the following: Not used when UseDefaultAddress is TRUE.
And previously, UseDefaultAddress
was always true.
// The spec does not seem to state if we need to call ServiceBinding->DestroyChild for | ||
// this handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reference implementation that does or doesn't do this? Or a way to check whether or not the handle is valid after this call?
@@ -15,7 +15,7 @@ pub(crate) struct Tcp4 { | |||
protocol: NonNull<tcp4::Protocol>, | |||
flag: AtomicBool, | |||
#[expect(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this lint still fire?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just tested
Add support for binding and accepting TCP4 connections. While testing, the following network options were used with QEMU + OVMF: -nic user,hostfwd=tcp::12345-:12345 The default localhost address on qemu seems to be 10.0.2.15. UEFI spec does not seem to state that the TCP Handle returned by the Accept method has a ServiceBinding Protocol. So have made the ServiceBinding Protocol optional. Signed-off-by: Ayush Singh <ayush@beagleboard.org>
54d3d51
to
5459fc4
Compare
Add support for binding and accepting TCP4 connections.
While testing, the following network options were used with QEMU + OVMF: -nic user,hostfwd=tcp::12345-:12345
The default localhost address on qemu seems to be 10.0.2.15.
UEFI spec does not seem to state that the TCP Handle returned by the Accept method has a ServiceBinding Protocol. So have made the ServiceBinding Protocol optional.
cc @nicholasbishop